Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Relax the lazy loading restrictions #37503

Merged
merged 1 commit into from
May 27, 2021
Merged

Conversation

themsaid
Copy link
Member

This PR will only throw the exception if the models being hydrated are more than 1. If it's only a single model then it means N in N+1 will always be 1. So no problem here.

@amcsi
Copy link

amcsi commented Jun 2, 2021

So to clarify, will this mean that...

$posts = Posts::limit(5)->get();
foreach ($posts as $post) {
	$comments = $post->comments;
}

...the above will trigger the lazy load protection, but...

$post = Posts::findOrFail(1);
$comments = $post->comments;

... will not?

Because then that's really cool.

@themsaid
Copy link
Member Author

themsaid commented Jun 2, 2021

@amcsi correct

@juliomotol
Copy link

I may be a bit late but we've noticed something and would like to clarify if it is the intended behavior given the ff cases:

Post has 5 Comment

$post = Posts::findOrFail(1);
$comments = $post->comments; // is allowed
$comments = $post->comments->first()->likes; // is not allowed

Getting Comment's related Like is not allowed since during hydrate() we found >1 $items

Post has only 1 Comment

$post = Posts::findOrFail(1);
$comments = $post->comments; // is allowed
$comments = $post->comments->first()->likes; // is allowed

Getting Comment's related Like is allowed since during hydrate() we only found 1 $items hence not preventing to lazy load

@Matslom
Copy link

Matslom commented Jan 16, 2023

So, when I set preventsLazyLoading and forgot specify with on query. Then on some results, when there will be more than one item I will get an exception, and on some not?
That feature leaks of consistency, don't you think? Potential bugs in code. If it works that way only real option to preventsLazyLoading is to set it only on local env...

Also, why that count($items) is inside array_map? Does every iteration need counting?

@amcsi
Copy link

amcsi commented Jan 16, 2023

@Matslom I agree with you. It would be better if PreventsLazyLoading didn't check whether there was more than 1 item in the collection for determining laziness violation, but rather if it was a result that came from a query that did not use ->first() or ->last(). But maybe it works this way, because there's no way to differentiate them.

In any case the way it works now is better than nothing. We get at least some warning/reminders about performance, even if it's a bit random.

@dennisprudlo
Copy link
Contributor

@Matslom

If it works that way only real option to preventsLazyLoading is to set it only on local env

You should use this feature only in local environments either way. This way you will be notified when there is some room for optimization.

I can't think of any added value when this is being used in a production environment except it is very critical to ones infrastructure that N+1 is prevented. As per the docs:

you may wish to only disable lazy loading in non-production environments so that your production environment will continue to function normally even if a lazy loaded relationship is accidentally present in production code

Even with consistent behavior you wouldn't want to present your user an error page just because you forgot some lazy-loading case.

@Matslom
Copy link

Matslom commented Jan 16, 2023

You should use this feature only in local environments either way

vs documentation

you may wish to only disable lazy loading in non-production environments so that your production environment will continue to function normally even if a lazy loaded relationship is accidentally present in production code

may wish !== should, because it may create problems over time

I'm creating app from scratch. First thing that I did was disablingLazyLoading. By reading the docs I didn't see case, why should I do this only on local env.
IMHO Docs should describe that "small" inconsistency created in this PR.

Even with consistent behavior you wouldn't want to present your user an error page just because you forgot some lazy-loading case.

If it would be consistent, then I don't see case when I ship some lazy-loading cases that can break over time.

@dennisprudlo
Copy link
Contributor

Of course you can enable it on production. Just saying I think there are very few cases where I would actually want to, because you throw a server error where there's actually no error. It's just inefficient code in most cases.

The app can still fallback to lazy loading without every breaking, like we log warnings instead of crashing the app intentionally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants